Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: digest with passing filters are not aggregated #4992

Merged
merged 2 commits into from
Dec 18, 2023

Conversation

ainouzgali
Copy link
Contributor

@ainouzgali ainouzgali commented Dec 17, 2023

What change does this PR introduce?

For a digest with filters that passes - it sent separate message for each trigger, instead of aggregating the events into one message.
Another problem fixed in this PR, is that a digest that is filtered (message should be sent immediately) was merged into a current delayed digest.

Why was this change needed?

Closes #4957

Other information (Screenshots)

@@ -33,6 +33,7 @@ export class QueueNextJob {
environmentId: command.environmentId,
organizationId: command.organizationId,
userId: command.userId,
job,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The conditions filter did not have the payload so all digests were created

@@ -207,6 +208,27 @@ export class JobRepository extends BaseRepository<JobDBModel, JobEntity, Enforce
}
}

if (filtered) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If filtered, we want a new digest to be created, and not merged to an existing delayed digest

},
{
$set: {
status: JobStatusEnum.DELAYED,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ainouzgali if this one was filtered out, why not marking it as skipped instead?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also then digestResult: DigestCreationResultEnum.SKIPPED should be updated ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can. But then we won't have the execution details. We will be having a different flow for digest than for the other steps.

Comment on lines +222 to +239
private async digestSkippedExecutionDetails(
job: JobEntity,
filtered: boolean
): Promise<void> {
const metadata = CreateExecutionDetailsCommand.getExecutionLogMetadata();
await this.executionLogQueueService.add(
metadata._id,
CreateExecutionDetailsCommand.create({
...metadata,
...CreateExecutionDetailsCommand.getDetailsFromJob(job),
detail: filtered ? DetailEnum.FILTER_STEPS : DetailEnum.DIGEST_SKIPPED,
source: ExecutionDetailsSourceEnum.INTERNAL,
status: ExecutionDetailsStatusEnum.SUCCESS,
isTest: false,
isRetry: false,
}),
job._organizationId
);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Show execution details for a skipped digest - for backoff as well. We only showed 'step created'

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great addition!

@ainouzgali ainouzgali requested a review from scopsy December 18, 2023 11:08
@ainouzgali ainouzgali merged commit 8dba437 into next Dec 18, 2023
29 checks passed
@ainouzgali ainouzgali deleted the fix-digest-with-filter-not-aggregated branch December 18, 2023 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 Bug Report: Digests with filter behave improperly
4 participants